Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spelling #480

Open
wants to merge 323 commits into
base: trunk
Choose a base branch
from
Open

Spelling #480

wants to merge 323 commits into from

Conversation

jsoref
Copy link

@jsoref jsoref commented Jun 23, 2023

This PR corrects misspellings identified by the check-spelling action.

The misspellings have been reported at https://github.com/jsoref/poi/actions/runs/5360105495#summary-14513021500

The action reports that the changes in this PR would make it happy: https://github.com/check-spelling/poi/actions/runs/5360101471#summary-14513009039

Copy link
Author

@jsoref jsoref left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the changes were automatically suggested by Google Sheets. A large portion of them were me applying other corrections or manually applying corrections. All fault mine.

// gradles gradle-worker.jar is still not a JPMS module and thus runs as unnamed module
// gradle's gradle-worker.jar is still not a JPMS module and thus runs as unnamed module
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

brand

build.gradle Outdated
// Some additional properties are currently set in the Jenkins-DSL, see jenksin/create_jobs.groovy
// Some additional properties are currently set in the Jenkins-DSL, see jenkins/create_jobs.groovy
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

brand

<echo file="download-snipplet.xml"><![CDATA[
<echo file="download-snippet.xml"><![CDATA[
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dunno if something cares about the filename...

LOG.atError().withThrowable(e).log("Batik is not not added to/working on the module-path. Use classpath mode instead of JPMS. Fallback to PNG.");
LOG.atError().withThrowable(e).log("Batik is not added to/working on the module-path. Use classpath mode instead of JPMS. Fallback to PNG.");
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doubled words have been historically flagged by spell checking tools... doubled negatives are problematic...

graphicFrame.setName("Diagramm" + frameId);
graphicFrame.setName("Diagram" + frameId);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this an api break?

"SecurityManager does not work any more since JDK 18");
"SecurityManager does not work since JDK 18");
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in principle, this should be anymore, but in practice, it's superfluous, so I removed it.

* not not issue an EOF when we discover the last block
* not to issue an EOF when we discover the last block
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or to not, but I think this is the right change...

// that that is now used if no policy given
// that it is now used if no policy given
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

atypical correction

@jsoref jsoref force-pushed the spelling branch 3 times, most recently from 014c79e to 5fa0530 Compare June 24, 2023 00:39
Comment on lines +306 to +307
assertEquals("Sheet1\nI have lots of embeded files in me\nSheet2\nSheet3\n", ex.getText()); /* fixing the spelling here requires editing 44643.xls */
assertEquals("Excel With Embeded", ex.getSummaryInformation().getTitle()); /* fixing the spelling here requires editing 44643.xls */
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm leaving these here. I'm not sure I have access to the right version of Excel to update the files. But I suspect that the rest of the changes for this commit are ok.

@jsoref jsoref marked this pull request as ready for review June 24, 2023 00:43
@jsoref jsoref requested a review from pjfanning June 24, 2023 00:43
@jsoref
Copy link
Author

jsoref commented Jun 24, 2023

It's possible everything else is ok. I'm done for the weekend. I'll check back sometime next week.

Thanks for the prompt response. I should have looked at the workflows more carefully, I missed that there was a pull_request: branches: trunk -- I'm used to CI that either doesn't exist (which is what I assumed based on the lack of any feedback in my personal PR) or that doesn't care about the target branch, in which case my personal PR would have triggered it and I would have worked w/ it until it was green before making this PR.

@jsoref
Copy link
Author

jsoref commented Jun 24, 2023

I've currently dropped spelling: secret, but I'm testing a fixed version of it in check-spelling-sandbox#2

@jsoref
Copy link
Author

jsoref commented Jun 25, 2023

I can't figure out where the setCurrentUserPasswort comes from...

      Test #309 document/PasswordProtected.doc HWPF FAILED (1.2s)

      org.opentest4j.AssertionFailedError: document/PasswordProtected.doc - failed. Message: document is encrypted, password is invalid - use Biff8EncryptionKey.setCurrentUserPassword() to set password before opening - didn't contain: document is encrypted, password is invalid - use Biff8EncryptionKey.setCurrentUserPasswort() to set password before opening ==> expected: <true> but was: <false>
          at app/org.apache.poi.stress/org.apache.poi.stress.TestAllFiles.verify(TestAllFiles.java:259)

Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1 from me

  • PR is much too big
  • changes data used in tests that can break tests
  • changes file names
  • changes API - we can't just remove a rename a public method - we need to deprecate it and add a new method with the corrected name - mark deprecations and since flags

I've applied a few low risk changes. It is not a priority for me to keep applying more.

jsoref added 15 commits June 27, 2023 08:00
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
jsoref added 26 commits June 27, 2023 08:00
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
@jsoref
Copy link
Author

jsoref commented Jun 27, 2023

Thanks, I'm happy to split out other pieces, e.g. perhaps things that don't rename files / apis / test data...

@pjfanning
Copy link
Contributor

Thanks @jsoref - I'm not 100% sure this is worth the effort. This is not even a live repo - the real code is in a subversion repo. So applying PRs to our code is very manual and time consuming.

Over time, I can come back to this PR and grab a couple more changes and apply them myself.

@jsoref
Copy link
Author

jsoref commented Jun 27, 2023

Yeah, I noticed the svn bits.

Anyway, thanks for working on this and the project in general. My employer uses this library :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants